Create a function to return the git-link itself #69

futuro · 2020-04-16T02:40:23Z

git-link is incredibly useful, but it doesn't return the URL it
creates. This introduces a new function which does so, allowing more
tooling to make use of this great feature.

This builds from the conversation in #38 and issue #37.

sshaw · 2020-04-17T04:03:52Z

Hi @futuro, thanks for the PR.

This is definitely something that's needed. I believe my concern with the other PRs was compatibility in that the signatures and/or return value must be changed in order to have a solid function and that calling git-link or whatever non-interactively should not add to the kill ring.

This certainly avoids that (yay) but what about the rest of the global state? For example, if you're using if for an org capture template then won't all the checks git-link-url is doing on the current buffer produce wrong or non-deterministic results?

futuro · 2020-04-22T16:15:25Z

Hey @sshaw,

I'll admit that I'm totally ignorant about global state (and emacs hacking in general), so I don't really have a good answer for this. I'll say that the function which is calling git-link-url is wrapped inside a with-current-buffer, so it's running (from what I understand) inside of the buffer for the file I want the git link for.

I'd welcome any pointers on what to read to understand the global state that's at play, or idiomatic design patterns for issues like this.

futuro · 2020-05-20T14:28:47Z

Thinking about this more, @sshaw is your question about whether git-link-url can be called as a pure function, versus needing to be called inside the buffer you want the link from?

sshaw · 2020-05-20T22:46:18Z

Hi @futuro, sorry for the holdup

is your question about whether git-link-url can be called as a pure function, versus needing to be called inside the buffer you want the link from?

Yes, I think that a standalone function should be used. Something like:

(git-link-url file-or-buffer &optional start end remote use-branch)

dotemacs · 2021-09-16T09:36:56Z

Hi @sshaw,

Thanks for the super useful package!

Is there a blocker for this feature to be merged? Thanks

sshaw · 2021-09-16T22:04:55Z

Is there a blocker for this feature to be merged? Thanks

It does not implement a standalone version of git-link-url (or similar). In its current form its return value depends on global state (unless I'm missing something?).

futuro · 2021-12-13T16:35:28Z

I'll see if I can shake that out today.

futuro · 2021-12-13T16:51:44Z

While I sort this out, @sshaw, I've noticed that git-link works in the same way as my change -- my change is just to extract some functionality from git-link -- and that much of git-link depends on global state, such as with an interactive call or within a with-current-buffer body.

My current thought is to have git-link-url contain a with-current-buffer call and have everything else be within that macro, but I'd love your thoughts on this and whether it's violating some kind of elisp design principle.

sshaw · 2021-12-14T01:20:49Z

Hi,

Thanks for getting back to this!

I've noticed that git-link works in the same way as my change ...
and that much of git-link depends on global state ...

The default case for git-link for interactive use, i.e., get a link of/based on the current buffer's contents and cursor position. This is okay.

My current thought is to have git-link-url contain a with-current-buffer call
... but I'd love your thoughts on this and whether it's violating some kind of elisp design principle.

This is where the problem lies. If it is going to be a legit stand-alone function it must not depend on things like the current buffer, cursor position, etc... Instead of being for interactive use it should have a signature similar to that outlined here and must return the URL and not add it to the kill ring.

I would say this is general principle for functions, etc...: don't depend on global state.

Once we have this baseline function it can be used for all use cases, including the interactive one.

Does that help?

sshaw · 2021-12-14T01:24:12Z

I would say this is general principle for functions, etc...: don't depend on global state.

Though we can depend on the output of git, which is sorta global state. If we didn't then we'd have more function arguments and I think the function would loose some of its utility then, as it would be forcing the caller to parse and pass in git output.

futuro · 2021-12-16T18:16:56Z

@sshaw after reviewing more of the code, I think I'm starting to see the way this could work. One question: what's the goal of the optional arg use-branch?

sshaw · 2021-12-16T18:51:43Z

... what's the goal of the optional arg use-branch?

That is supposed to be the equivalent of sorts of git-link-use-commit. Currently the default is to generate the link using the branch but time has shown that it's better to use the commit. So I think eventually git-link-use-commit will be git-link-use-branch to better reflect this. This argument is named to reflect that.

futuro · 2021-12-16T19:03:33Z

That is supposed to be the equivalent of sorts of git-link-use-commit.

Rockin, that makes sense.

Another question! You'd put the remote as an optional argument, but the function can't work without a specified remote (as far as I understand it, at least). I'm thinking about moving this to be a required argument; am I missing anything?

futuro · 2021-12-16T19:35:14Z

Ok @sshaw, this should be ready.

I went ahead with making remote a required argument, but let me know if I missed something and it should be optional.

futuro · 2021-12-16T19:42:18Z

J/k, sorry. I need to fix a void-variable reference. Will ping you once it's solid.

futuro · 2021-12-16T19:51:00Z

Ok, tested, and should be good to go for review @sshaw.

sshaw · 2021-12-22T22:02:48Z

Thanks a lot. I will have a looksy at or before Christmas Day 🎅🤶

git-link.el

sshaw · 2021-12-25T01:49:28Z

Hi, @futuro, lookin' good. Aside from my 2 comments I would squash this PR into a single commit.

Merry Christmas! 🎄

futuro · 2022-01-05T17:03:32Z

Hey @sshaw, thanks for the suggestions on user-error! I've never done error signaling/handling in elisp, so that was a fun excursion.

I've just pushed up that change, and it should be ready for review.

Just to clarify, are you asking me to squash all the commits in this PR, or are you saying that you're going to do a squash merge afterwards?

Thanks! And Happy New Year!

sshaw · 2022-01-05T22:51:42Z

Just to clarify, are you asking me to squash all the commits in this PR, or are you saying that you're going to do a squash merge afterwards?

Squash all commits, please.

futuro · 2022-01-11T20:12:46Z

@sshaw commits are squashed and everything should be ready to go.

sshaw

Hi, thanks for squashing.

I had an in-depth look now and there are a few issues. See comments.

sshaw · 2022-01-15T16:16:57Z

git-link.el

+                    (if (bufferp file-or-buffer)
+                        (buffer-file-name file-or-buffer)
+                      file-or-buffer)))
+         (remote-url (git-link--remote-url remote))


We must check for a nil remote-url first, like we do here: https://github.com/sshaw/git-link/pull/69/files#diff-a86ea92b0e180064b421a637b78c7d5ae152de93c60a82136b77a6a72c90b35cL692
Otherwise we get errors when the remote does not exist.

A case where the remote does not exist would be trying to link from a buffer that is not part of a git repo.

git-link.el

sshaw · 2022-01-15T16:26:53Z

git-link.el

@@ -666,6 +666,54 @@ return (FILENAME . REVISION) otherwise nil."
    (git-link--remote)))

 ;;;###autoload
+(defun git-link-url (file-or-buffer remote &optional start end use-branch-p)


Some thoughts/issues here:

I think we should default remote to git-link-default-remote.

If the current buffer is in directory X but this is called like (git-link-url "/path/to/Y/foo.txt" "origin" 10), where Y is a different repository root, it should return a link to Y/foo.txt's repo but now it links to something random in repo X.

What to do with invalid line numbers, e.g., when end is after or equal to start, when start is nil and end is given? Thinking we check for these cases and signal an error.

I agree with items 1 and 3, but item 2 strikes me as out of scope for this PR. Specifically, my first thought for handling this is to change every function that interacts with git-link--exec such that they can pass a buffer/directory to run from, thus isolating the side-effect of changing our working directory.

Do you see another way of achieving 2 that only impacts git-link-url?

... but item 2 strikes me as out of scope for this PR.

When calling it as a separate function it cannot depend on state of some buffer. For example, if one creates an emacs package that calls git-link-url the call will depend on the state of some git repo outside the function call so bugs would occur. Make sense?

I agree with items 1 and 3,

Maybe 2/3 is fine? Do they togeather outweigh the bug?

Do you see another way of achieving 2 that only impacts git-link-url?

I think this is part of the reason why this function has taken so long to exist. If I remember correctly you have to change a bit more than meets the eye. It has been since Dec or so since I had a look at the code. Will have to wait to look again. But if you think you gotz a solution then add it. But if you are concerned about your time then maybe wait 'till I can have an in-depth look again.

Also test coverage not that good. Exhibit A.

`git-link` is incredibly useful, but it doesn't return the URL it creates. This introduces a new function which does so, allowing more tooling to make use of this great feature. Wrapped up in this I did some refactoring to drop the use of `setq` internally to the function, as that defies the purely functional nature of the call. This _shouldn't_ break anything.

sshaw · 2023-01-08T13:52:00Z

Closing to due lack of feedback/progress.

sshaw mentioned this pull request Apr 17, 2020

Return link #38

Closed

sshaw added the more info label Apr 18, 2020

sshaw removed the more info label May 22, 2020

sshaw mentioned this pull request Sep 6, 2020

feat: git-link-blame #70

Closed

sshaw reviewed Dec 25, 2021

View reviewed changes

git-link.el Outdated Show resolved Hide resolved

sshaw reviewed Dec 25, 2021

View reviewed changes

git-link.el Outdated Show resolved Hide resolved

futuro force-pushed the master branch 2 times, most recently from 84da7f4 to 26ca72c Compare January 11, 2022 20:11

sshaw requested changes Jan 15, 2022

View reviewed changes

futuro force-pushed the master branch from 26ca72c to de8cf38 Compare March 16, 2022 15:55

futuro force-pushed the master branch from de8cf38 to 68d8420 Compare March 16, 2022 16:19

sshaw closed this Jan 8, 2023

sshaw mentioned this pull request Mar 3, 2024

How to call git-link from another interactive function? #118

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a function to return the git-link itself #69

Create a function to return the git-link itself #69

futuro commented Apr 16, 2020

sshaw commented Apr 17, 2020

futuro commented Apr 22, 2020

futuro commented May 20, 2020

sshaw commented May 20, 2020

dotemacs commented Sep 16, 2021

sshaw commented Sep 16, 2021

futuro commented Dec 13, 2021

futuro commented Dec 13, 2021

sshaw commented Dec 14, 2021

sshaw commented Dec 14, 2021 •

edited

Loading

futuro commented Dec 16, 2021

sshaw commented Dec 16, 2021 •

edited

Loading

futuro commented Dec 16, 2021

futuro commented Dec 16, 2021

futuro commented Dec 16, 2021

futuro commented Dec 16, 2021

sshaw commented Dec 22, 2021

sshaw commented Dec 25, 2021

futuro commented Jan 5, 2022

sshaw commented Jan 5, 2022

futuro commented Jan 11, 2022

sshaw left a comment

sshaw Jan 15, 2022

sshaw Jan 15, 2022

sshaw Jan 15, 2022

futuro Mar 16, 2022 •

edited

Loading

sshaw Mar 20, 2022

sshaw Mar 20, 2022

sshaw commented Jan 8, 2023

Create a function to return the git-link itself #69

Create a function to return the git-link itself #69

Conversation

futuro commented Apr 16, 2020

sshaw commented Apr 17, 2020

futuro commented Apr 22, 2020

futuro commented May 20, 2020

sshaw commented May 20, 2020

dotemacs commented Sep 16, 2021

sshaw commented Sep 16, 2021

futuro commented Dec 13, 2021

futuro commented Dec 13, 2021

sshaw commented Dec 14, 2021

sshaw commented Dec 14, 2021 • edited Loading

futuro commented Dec 16, 2021

sshaw commented Dec 16, 2021 • edited Loading

futuro commented Dec 16, 2021

futuro commented Dec 16, 2021

futuro commented Dec 16, 2021

futuro commented Dec 16, 2021

sshaw commented Dec 22, 2021

sshaw commented Dec 25, 2021

futuro commented Jan 5, 2022

sshaw commented Jan 5, 2022

futuro commented Jan 11, 2022

sshaw left a comment

Choose a reason for hiding this comment

sshaw Jan 15, 2022

Choose a reason for hiding this comment

sshaw Jan 15, 2022

Choose a reason for hiding this comment

sshaw Jan 15, 2022

Choose a reason for hiding this comment

futuro Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

sshaw Mar 20, 2022

Choose a reason for hiding this comment

sshaw Mar 20, 2022

Choose a reason for hiding this comment

sshaw commented Jan 8, 2023

sshaw commented Dec 14, 2021 •

edited

Loading

sshaw commented Dec 16, 2021 •

edited

Loading

futuro Mar 16, 2022 •

edited

Loading